-
Notifications
You must be signed in to change notification settings - Fork 888
completed-docs: Limit variable checking to file-level variables and use correct visibility #2950
Conversation
src/rules/completedDocsRule.ts
Outdated
private checkVariable(node: ts.VariableDeclaration) { | ||
// Only check variables in variable declaration lists | ||
// and not variables in catch clauses and for loops. | ||
if (node.parent === undefined || node.parent.kind !== ts.SyntaxKind.VariableDeclarationList) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node.parent
as well as list.parent
and statement.parent
will never be undefined. You can simply assert that using for example node.parent!.kind
src/rules/completedDocsRule.ts
Outdated
// Only check variables at the file-level and not | ||
// variables declared inside functions and other things. | ||
const statement = list.parent; | ||
if (statement.parent === undefined || statement.parent.kind === ts.SyntaxKind.SourceFile) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about (exported) variables inside a namespace (ts.SyntaxKind.ModuleBlock
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point 👍
Just pushed up some changes suggested by @ajafff. |
src/rules/completedDocsRule.ts
Outdated
|
||
// Only check variables at the namespace/module-level or file-level | ||
// and not variables declared inside functions and other things. | ||
let check = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
statement.parent
can not be undefined. This check ccould be simplified to
switch (statement.parent!.kind) {
case ts.SyntaxKind.SourceFile:
case ts.SyntaxKind.ModuleBlock:
break;
default:
return;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doh! Sorry, missed that on your previous comment. Am I correct in assuming it's defined as parent?: Node
because it can be undefined if the setParentNodes
argument is false
when you load the TypeScript source file. But since TSLint always passes true
to that argument, the parent nodes will always be set (except on the root SourceFile
node)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly how it works.
…s and use correct visibility.
Thanks @reduckted |
…s and use correct visibility. (palantir#2950)
PR checklist
Overview of change:
This PR fixes two issues:
SourceFile
- i.e. variables at the file-level.As a side note, I've also fixed the
.editorconfig
file to have a rule for tslint.json files that enforces an indent size of 2 instead of the default 4, because all tslint.json files I could see used an indentation of 2.CHANGELOG.md entry:
[bugfix]
completed-docs
: Uses correct visibility of variables.[bugfix]
completed-docs
: Only checks variables at the file-level.